Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace deprecated binder2nd and binder1st #1287

Merged
merged 5 commits into from
Nov 7, 2017

Conversation

phcerdan
Copy link
Member

@phcerdan phcerdan commented Nov 1, 2017

binder2nd generates error in c++17

Replace by lambdas when possible and with std::function when the type is
required.

decltype(lambda) does not work, so we bind the lambda to a std::function.

Other option also used is std::bind and std::placeholders.

FIX #1221

Checklist

  • [NA] Unit-test of your feature with Catch.
  • [NA] Doxygen documentation of the code completed (classes, methods, types, members...)
  • [NA] Documentation module page added or updated.
  • New entry in the ChangeLog.md added.
  • No warning raised in Debug cmake mode (otherwise, Travis C.I. will fail).
  • All continuous integration tests pass (Travis & appveyor)

binder2nd generates error in c++17

Replace by lambdas when possible and with std::function when the type is
required.

decltype(lambda) does not work, so we bind the lambda to a std::function.

Other option is to use std::bind and std::placeholders, but it is harder to read the code IMO.

FIX DGtal-team#1221
@rolanddenis
Copy link
Member

Thanks !

In fact, we can make a decltype on an already-existing lambda but compilation will fail due to the Functor concept in DGtal that except a functor to be assignable. Since a lambda is not assignable, I agree that using std::function is the best way !

There is still one std::binder1st left in DGtal/graph/DistanceBreadthFirstVisitor.h :octocat:

using ValuePredicate = std::function<bool(Image::Value)>;
ValuePredicate equalTo1 = [](Image::Value&& v)
{
return std::forward<Image::Value>(v) == 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also simply pass v by const-ref and test v == 1. In fact, using && here forbids lvalue parameters, like in the code

const auto fn = [] ( int&& v ) { return v == 1; };
int a = 3;
bool dummy = fn(a);

Here, since the std::function signature specifies a pass-by-value parameter, it doesn't fail.

Therefore, I also suggest to use a std::function<bool(Image::Value const&)>.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, makes sense, still rusty with std::forward usage. I will change it in 12 hours

@phcerdan
Copy link
Member Author

phcerdan commented Nov 2, 2017

Good points, I will polish it tomorrow morning, thanks!

@dcoeurjo
Copy link
Member

dcoeurjo commented Nov 2, 2017

(tagged as 0.9.4)

@dcoeurjo dcoeurjo added this to the 0.9.4 milestone Nov 2, 2017
@dcoeurjo dcoeurjo self-assigned this Nov 2, 2017
Use std::bind instead of lambdas in examples.
@phcerdan
Copy link
Member Author

phcerdan commented Nov 2, 2017

I have changed the signature of std::function as @rolanddenis suggested.

Replaced binder1st with std::function, and its instantiation, using std::bind1st, with a std::bind and std::placeholder.

@phcerdan phcerdan changed the title Replace deprecated binder2nd Replace deprecated binder2nd and binder1st Nov 2, 2017
@rolanddenis
Copy link
Member

Thanks !
I have also removed useless boost/bind.hpp includes.

@dcoeurjo
Copy link
Member

dcoeurjo commented Nov 7, 2017

thanks @phcerdan & @rolanddenis

Merging..

@dcoeurjo dcoeurjo merged commit a81712e into DGtal-team:master Nov 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants